chore(logger): make Logger E2E works for all runtimes #559

ijemmy · 2022-02-15T14:08:13Z

Description of your changes

Currently, E2E only runs on a single (default) runtime from CDK Lambda module. With this change,test:e2e target runs tests in all runtime to detect any bugs from compatibility issue. It will create 1 stack of test Lambda functions per run time and execute the tests in parallel.

NodeJS10 has been deprecated. So we will be running on v12 and v14 for now.

Why this approach?

There are several options to achieve this. Here are the main criteria in selecting an approach.

Speed Able to deploy infrastructure (in beforeAll() block) concurrently.
Cognitive load for test writers Test writers can focus more on testing and don't get distracted by having to think of multiple runtimes.
Easy to read the log Logs from concurrent test runs (from multiple runtimes) will overlap with each other.

Here are the list of options I evaluated:

Use it.each() and describe.each() to iterate an array of runtimes. (link) This option doesn't work as Jest won't parallel the beforeAll() execution in the same file. It doubles the time to run the tests. In addition, test writers have to explicitly specify an array of run-time, more boilerplate code.
Have a custom script run-e2e.ts that starts subprocess to run concurrently . This works with Node's child_process's exec. However, the log coloring is lost. In additional, we need to maintain an additional script.
Use forEach to iterate describe blocks to run each runtime test independently. This also won't run beforeAll() in parallel. Jest that decides to run things in parallel or sequential. We have no control over it. describe and it basically put things in an internal queue and jest has some heuristic algorithm to decide. I find that it stops running in parallel when when several describe are called in the same file.
Use concurrently and pass runtime as an environment variable. I find this option cleanest and satisfy the 3 criteria above. The logs overlap with each other but we there is a prefix stating which runtime the log comes from.

How to verify this change

Check out this branch and run

cd packages/logger
AWS_PROFILE=<YOUR_PROFILE> AWS_REGION=eu-central-1 npm run test:e2e

Result from local run

[test:e2e:nodejs12x]
[test:e2e:nodejs12x] Test Suites: 3 passed, 3 of 8 total
[test:e2e:nodejs12x] Tests:       13 passed, 13 total
[test:e2e:nodejs12x] Snapshots:   0 total
[test:e2e:nodejs12x] Time:        115.722 s, estimated 136 s
[test:e2e:nodejs12x] Ran all test suites.
[test:e2e:nodejs12x] npm run test:e2e:nodejs12x exited with code 0
[test:e2e:nodejs14x]
[test:e2e:nodejs14x] Test Suites: 3 passed, 3 of 8 total
[test:e2e:nodejs14x] Tests:       13 passed, 13 total
[test:e2e:nodejs14x] Snapshots:   0 total
[test:e2e:nodejs14x] Time:        115.771 s, estimated 136 s
[test:e2e:nodejs14x] Ran all test suites.
[test:e2e:nodejs14x] npm run test:e2e:nodejs14x exited with code 0

Related issues, RFCs

#353

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

Breaking change checklist

I have documented the migration process
I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

ijemmy · 2022-02-15T14:52:49Z

E2E test failed due to stack creation timeout: https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/1847481128

I increased stack timeout to 300 seconds

ijemmy · 2022-02-15T15:33:42Z

E2E tests pass: https://github.com/awslabs/aws-lambda-powertools-typescript/runs/5202416034?check_suite_focus=true

flochaz

LGTM but another option would have been to leverage github actions matrix

dreamorosi · 2022-02-15T15:48:30Z

LGTM but another option would have been to leverage github actions matrix

That's a good point, it seems that the example shown in this section of the docs would be what we are looking for:

name: Node.js CI
on: [push]
jobs:
  build:
    runs-on: ubuntu-latest
    strategy:
      matrix:
       include:
         - node-version: 10.x
           site: "prod"
           datacenter: "site-a"
         - node-version: 12.x
           site: "dev"
           datacenter: "site-b"
    steps:
      - name: Echo site details
        env:
          SITE: ${{ matrix.site }}
          DATACENTER: ${{ matrix.datacenter }}
        run: echo $SITE $DATACENTER

The next section about concurrency also says that it's possible to specify concurrency, the only downside of this is that there's no guarantee that jobs will run in parallel as this is decided based on availability of the runners.

If we are okay with this then we could avoid bringing an additional dependency into the project (concurrently@^7.0.0) which in the long term could save us some headaches / overhead.

Other than this I like how you implemented it and the helpers / constants that you have added.

…ar to e2e tests

This reverts commit 85a67ab.

…ss env var to e2e tests" This reverts commit f11140c.

ijemmy · 2022-02-16T14:10:41Z

@flochaz @dreamorosi I have tried using matrix strategy. However, it breaks the e2e tests in metrics package as two two set of e2e tests for 2 node runtimes are running together with the same stack name. https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/1853018538

I propose to go with concurrently for now. Then I'll create PRs to make stack id unique (+ refactor them to be similar to logger). Then I'll make another PR to use strategy matrix. Is that ok with you?

dreamorosi

@ijemmy thanks for trying out the other method, your proposal is fine by me and I'd prioritise in this order:

having e2e test for all utils
having all utils running on all runtimes
optimising test harness

So with that said no need to tackle this immediately, thanks a ton for considering it tho!

chore(logger): make E2E works for all runtimes

d4bdf1b

github-actions bot added the internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) label Feb 15, 2022

chore(logger): use generateUniqueName function

9f20c43

ijemmy requested review from flochaz, dreamorosi and saragerion February 15, 2022 14:20

chore(logger): Increase e2e stack creation timeout to 300 seconds

df415e1

ijemmy marked this pull request as ready for review February 15, 2022 15:33

flochaz previously approved these changes Feb 15, 2022

View reviewed changes

chore(logger): Test using Github Action matrix strategy to pass env v…

f11140c

…ar to e2e tests

ijemmy dismissed flochaz’s stale review via f11140c February 16, 2022 13:11

ijemmy added 3 commits February 16, 2022 14:22

chore(logger): Fix e2e test with strategy matrix

85a67ab

Revert "chore(logger): Fix e2e test with strategy matrix"

564f4b4

This reverts commit 85a67ab.

Revert "chore(logger): Test using Github Action matrix strategy to pa…

d655567

…ss env var to e2e tests" This reverts commit f11140c.

dreamorosi approved these changes Feb 16, 2022

View reviewed changes

flochaz approved these changes Feb 16, 2022

View reviewed changes

dreamorosi assigned ijemmy Feb 16, 2022

dreamorosi added this to the production-ready-release milestone Feb 16, 2022

ijemmy merged commit 7c14ce7 into main Feb 16, 2022

ijemmy deleted the chore/run-e2e-multiple-runtimes branch February 16, 2022 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(logger): make Logger E2E works for all runtimes #559

chore(logger): make Logger E2E works for all runtimes #559

ijemmy commented Feb 15, 2022 •

edited

Loading

ijemmy commented Feb 15, 2022

ijemmy commented Feb 15, 2022

flochaz left a comment

dreamorosi commented Feb 15, 2022

ijemmy commented Feb 16, 2022

dreamorosi left a comment

chore(logger): make Logger E2E works for all runtimes #559

chore(logger): make Logger E2E works for all runtimes #559

Conversation

ijemmy commented Feb 15, 2022 • edited Loading

Description of your changes

Why this approach?

How to verify this change

Related issues, RFCs

PR status

Checklist

Breaking change checklist

ijemmy commented Feb 15, 2022

ijemmy commented Feb 15, 2022

flochaz left a comment

Choose a reason for hiding this comment

dreamorosi commented Feb 15, 2022

ijemmy commented Feb 16, 2022

dreamorosi left a comment

Choose a reason for hiding this comment

ijemmy commented Feb 15, 2022 •

edited

Loading